feat: enhance logging with debug timing for various operations and ad…#631
Merged
feat: enhance logging with debug timing for various operations and ad…#631
Conversation
…d well export endpoint
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses an API lock-up observed after PDF generation by making GET /sample/{id} deterministic via explicit eager-loading of the SampleResponse relationship graph, while refining request lifecycle logging and gating detailed timing instrumentation behind API_DEBUG_TIMING.
Changes:
- Eager-load
Samplerelationships for both list and detail sample retrieval, and routeGET /sample/{id}through a dedicated helper. - Add lightweight request lifecycle logs (
request started/request completed) and gate detailed timing logs across several services withAPI_DEBUG_TIMING. - Update dev runtime defaults (Docker entrypoint / compose) and add/adjust tests for logging and GCS upload behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
core/app.py |
Replaces request timing lock logic with request lifecycle logging and request IDs. |
api/sample.py |
Switches sample-by-id to a helper that eagerly loads the response graph. |
services/sample_helper.py |
Introduces comprehensive loader options and a new “by id with relationships” query helper. |
api/asset.py |
Adds debug timing for bucket resolution/upload; moves signed-url generation to a threadpool for single-asset fetch. |
services/gcs_helper.py |
Adds chunked hashing, stage timing logs (gated), and additional stage instrumentation. |
services/thing_helper.py |
Adds optional (gated) timing log for thing lookup. |
services/observation_helper.py |
Adds optional (gated) timing log for observation pagination query. |
services/well_details_helper.py |
Adds (gated) stage timing logs and a new export payload builder. |
api/thing.py |
Adds a new water-well export endpoint returning the minimal export payload. |
schemas/well_export.py |
Adds WellExportResponse schema for the new export endpoint. |
db/engine.py |
Adds connection pool event logging and configurable DB_POOL_TIMEOUT. |
tests/test_request_timing.py |
Updates tests to assert lifecycle logs instead of cold/warm request timing logs. |
tests/test_asset.py |
Adds tests for GCS timing logs, skipping existing blobs, and file rewind behavior; minor assertion/style tweaks. |
entrypoint.sh |
Makes Uvicorn --reload opt-in via UVICORN_RELOAD=true. |
docker-compose.yml |
Switches DB service to a PostGIS image and tweaks port quoting/commented build config. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65e51c6678
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hance debug timing functionality
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the API lock-up that showed up after generating a PDF from the Field Compilation sheets flow, and trims the temporary debugging instrumentation down to a maintainable baseline.
The main issue ended up being on the follow-up read path, not the export/upload itself. After the initial export completed, concurrent
GET /sample/{id}requests could stall or fail due to ORM relationship loading behavior. This PR makes the sample endpoint load the response graph explicitly and safely, while also keeping the request lifecycle visible in logs.What Changed
GET /sample/{id}to use an eager-loaded query instead of returning a lazily-loaded ORM object.SampleResponserelationships up front.request startedrequest completedAPI_DEBUG_TIMING=truein:uvicornno longer defaults to--reload, with opt-in reload viaUVICORN_RELOAD=true.Why
The original symptom looked like “PDF generation breaks the API,” but the logs showed the export, asset lookup, and groundwater observation requests were completing. The hang began when the frontend issued follow-up sample requests. Those responses were relying on ORM relationship loading during serialization, which is brittle under concurrent access and was enough to wedge the worker in this flow.
This change makes the sample endpoint deterministic and removes most of the high-volume timing noise unless explicitly enabled for debugging.
Operational Notes
API_DEBUG_TIMING=trueto re-enable detailed timing/stage logs.UVICORN_RELOAD=trueif you want reload behavior in local Docker dev.Validation
blackpassed on touched files.python -m py_compilepassed on touched files.flake8still reports pre-existing line-length (E501) issues in several touched modules.pytestwas not run in this pass.